-
Notifications
You must be signed in to change notification settings - Fork 4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix imports not being removed from MAS files. #44230
Conversation
tagging @dotnet/roslyn-ide |
using Microsoft.CodeAnalysis.Formatting.Rules; | ||
using Roslyn.Utilities; | ||
|
||
namespace Microsoft.CodeAnalysis.CSharp.MetadataAsSource |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is justa move.
@@ -71,55 +73,162 @@ protected override ImmutableArray<AbstractReducer> GetReducers() | |||
new CSharpParenthesizedPatternReducer(), | |||
new CSharpDefaultExpressionReducer()); | |||
|
|||
private class FormattingRule : AbstractMetadataFormattingRule |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this type was moved to a sibling file.
src/Features/CSharp/Portable/MetadataAsSource/CSharpMetadataAsSourceService.cs
Show resolved
Hide resolved
@CyrusNajmabadi Does this also close #30327? |
@CyrusNajmabadi So the extra using directives were being added because we were always adding Attribute namespaces to the list of usings, and the .NET 5 assemblies now have nullable annotations? |
Yup! |
That was one problem. But the primary problem was that we basically turned off the step to then clean up those imports. So any added imports that turned out to be unnecessary just were not removed. Nullable just exacerbated this as you might have a ton of those in code which just exploded this sort of thing. |
|
||
[NullableContextAttribute(1)] | ||
public interface [|C|]<[NullableAttribute(2)] T> | ||
public interface [|C|]<T> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this have gotten a #nullable enable
added?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not that i can tell. @jcouv we dont' need #nullable enable
here right? we don't have an annotated reference type afaict.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The attribute value represents the lack of a notnull
constraint - see examples.
src/Workspaces/Core/Portable/CodeGeneration/NullableSyntaxAnnotation.cs
Outdated
Show resolved
Hide resolved
...Workspaces/SharedUtilitiesAndExtensions/Workspace/CSharp/Extensions/ITypeSymbolExtensions.cs
Show resolved
Hide resolved
...Workspaces/SharedUtilitiesAndExtensions/Workspace/CSharp/Extensions/ITypeSymbolExtensions.cs
Outdated
Show resolved
Hide resolved
src/Workspaces/CSharp/Portable/CodeGeneration/AttributeGenerator.cs
Outdated
Show resolved
Hide resolved
// if we hit a type, and we're currently disabled, then switch us back to enabled for that type. | ||
// This ensures whenever we walk into a type-decl, we're always in the enabled-state. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is that to simply the algorithm since we're doing this as a bottom-up rewrite so we can't really know where the type might end up?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
that's exactly it. it also keeps things stateless so we don't need to keep track of everything as we walk.
src/Features/CSharp/Portable/MetadataAsSource/CSharpMetadataAsSourceService.cs
Outdated
Show resolved
Hide resolved
{{ | ||
public TestType(); | ||
|
||
#nullable disable |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When looking at the implementation at first this seemed a bit strange to tag it this way but I have to say I think I actually like it -- it means there's always a #nullable enable at the top which is easy to see, and the special cases are called out specifically.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yup. that seemed really sane and sensible. it's the coding pattern i would prefer to see tbh :)
src/Features/CSharp/Portable/MetadataAsSource/CSharpMetadataAsSourceService.cs
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Auto-approval
{ | ||
} | ||
|
||
#nullable disable |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: formatting
public TestType(); | ||
|
||
public void [|M1|](dynamic s); | ||
}}"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider including a nasty case (where best effort fails):
List<string
#nullable enable
> M(
#nullable disable
string parameter) => throw null;
Also, the case we discussed yesterday where members omitted by MAS still affect the output.
Fixes #44200
Fixes #30327
Also does a reasonable effort of emitting
#nullable enable/disable
directives in MAS files when it contains signatures wiht types that are annotated/oblivious.